Add categories for alternative image types that can be decoded using registered custom decoders #602
Conversation
…registered custom decoders
…registered custom decoders
There was a problem hiding this comment.
I'm glad to see this direction! One thing that we should probably think about is priority? I could imagine formats that could potentially be decoded by more than one provider? In that case it would be indeterminate which one would be used? Maybe I'm thinking too far ahead / over complicating things…
Excited to introduce this to PINImage+DecodedImage too!
| + (void)pin_registerCustomDecoder:(id<PINImageCustomDecoder>)customDecoder { | ||
| NSMutableArray<id<PINImageCustomDecoder>> *decoders = [PINImage pin_decoders]; | ||
| [decoders addObject:customDecoder]; | ||
| objc_setAssociatedObject(self, @selector(pin_registerCustomDecoder:), decoders, OBJC_ASSOCIATION_RETAIN_NONATOMIC); |
There was a problem hiding this comment.
Any reason to be using nonatomic here and below? Especially for the class method this seems like it could result in race conditions?
There was a problem hiding this comment.
Ah good question! It's because I actually do the locking further up the call chain internally where this is used, but your point totally makes sense for more general use
Do you prefer using the atomic keyword or some other locking mechanism in PINRemoteImage?
The priority point is a good one as well. A couple options:
- Can simply add a comment that in the case where multiple decoders can handle a given type, the first decoder that was registered will be used (current behavior)
- Can have decoders self declare their priority with a new property on PINImageCustomDecoder (in which case there may still be decoders with equal priorities)
- Can have pin_decodedImageUsingCustomDecodersWithSize centrally decide some policy on how to assess priority
WDYT?
There was a problem hiding this comment.
For associated objects, atomic seems like the easiest in this case and it's used elsewhere in the framework.
I think I like # 2 for the options you presented with a comment about equal priorities. I think that would give enough flexibility without complicating the API too much?
Thanks again for thinking all this through!
There was a problem hiding this comment.
After some internal discussion I updated the PR to remove the custom decoder registry from PINRemoteImage for now and instead change it to a property with a single decoder to be used that is assigned from outside of PINRemoteImage from the place that invokes this logic. In other words, the decision for which decoder to use for the encoded data is the responsibility of the caller, which simplifies the logic for now.
As far as the locking, I changed the associated object setting to be RETAIN instead of RETAIN_NONATOMIC
The thinking is that we could defer the final design of the decoder registry until we actually have a full solution for the remote image scenario, since for now in our solution we'd like the selection of the decoder to be context-specific, and it's not yet clear how to achieve this in PINRemoteImage which doesn't have knowledge of such contexts
How does that sound?
There was a problem hiding this comment.
Just wanted to bump this!
…ing a single supplied custom decoder to be assigned
…i-google/PINRemoteImage into alternative-image-types
This establishes a general pattern in order to register additional image decoders that can handle image formats not natively supported on iOS/MacOS
Upstreamed contribution from Google's fork of PINRemoteImage. Our immediate use case was for SVG image support using internal SVG decoding libraries
This PR only introduces the protocols -- our immediate use case was for inlined image bytes and thus no changes were made yet to PINImage+DecodedImage -- but we plan to add additional support for remote images and contribute that change back to this Github later this year
See TextureGroup/Texture#2011 for context on where this change is used